Skip to content

Conversation

@chamilo-uga
Copy link
Contributor

See #6344

the goal is to allow teachers to quickly have a full users list page of members of a class registered to a course.
It is a very simple and basic overview page with

list of users and a search bar.
It's voluntary on our part to have a sober large vertically scrolled page with no jqGrid and just a simple search bar on top.
There are 2 lists

    Students in class registered in course
    Students in class unregistered in course(*)

(*) a teacher can unregister from a course a student in a registered class in course.

We have information
    student firstname + lastname and email
    because users dont often know the username of a user in platform.
    It is easier for them to use email to check for homonymous

@chamilo-uga chamilo-uga changed the title PR#6344 - In Course>Users>Usergroups, add a action button for usergroups to have the preview list members #6344 - In Course>Users>Usergroups, add a action button for usergroups to have the preview list members Oct 8, 2025
}
}

public function unsubscribe_only_courses_from_usergroup($usergroup_id, $delete_items, $sessionId = 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHPDoc block needed to document the method

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method should provide a return type

Copy link
Member

@ywarnier ywarnier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, please try using even more the code from src/ rather than using code from legacy libraries. This will help us move faster to remove/migrate all legacy code.

Please see and attend change requests before we merge.

$groupId = isset($_GET['id']) ? (int) $_GET['id'] : 0;
foreach ($delete_items as $course_id) {
$course_info = api_get_course_info_by_id($course_id);
if ($course_info) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inefficient. api_get_course_info_by_id() is a heavy function. Please just replace by a validation that $course_id is an int, and that should be enough (worst case: the course_id is not present in usergroup_rel_course and the delete fails but does not break the process).

);
}
if (0 != $sessionId && 0 != $groupId) {
$this->subscribe_sessions_to_usergroup($groupId, [0]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot add a "subscribe*()" call inside a method that is called "unsubscribe_only_courses_from_usergroup()". What is the goal here ?

if (0 != $sessionId && 0 != $groupId) {
$this->subscribe_sessions_to_usergroup($groupId, [0]);
} else {
$s = $sessionId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This $s is never used. It should either be removed or used and documented.

@@ -0,0 +1,72 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a Helper

@@ -0,0 +1,30 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code should go into a new file called UsergroupController.php rather than ClassController.php (we don't use the term "Class" in the code anymore).

/**
* @author Julio Montoya <[email protected]>
*/
#[Route('/user')]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing /user is not a super nice idea. Better leave it and use /user/overview as path for overview().
Paths with "main" are only kept until all the legacy code is migrated to Vue

@@ -0,0 +1,98 @@
{% extends "@ChamiloCore/Layout/base-layout.html.twig" %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move to src/CoreBundle/Resources/views/Usergroup/overview.html.twig

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants